Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add supported platform section to README #481

Merged
merged 4 commits into from
Apr 5, 2018

Conversation

funkyboy
Copy link
Contributor

@funkyboy funkyboy commented Mar 2, 2018

No description provided.

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start. However, in this case I think we need to add a statement like:

The ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR.

Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks.

These mean that the library should be compatible with nearly any recent browser, on the majority of platforms.

If, however, you do find compatibility issues with any specific platform and browser combination, please raise an issue and/or contact customer support. Known compatibility issues can be found at ....

README.md Outdated

**Note**: the ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR.
Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks.
These mean that the library should be compatible with nearly any recent browser, on the majority of platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compatible with nearly any recent browser

Do we not mean nearly every browser being used? Recent makes it sound like we're only supporting ones released in the last few months / year at best

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just remove "recent"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just remove "recent"

We still need to do this

README.md Outdated
|   8 | Firefox 58+ |
|   9   | Safari macOS 11 |
|       | Safari iOS 11 |
|   | Chrome on Android 6|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly restrictive -- I think we're underselling ourselves.

E.g. it makes it sound like we don't support firefox <=57, which may be rather off-putting for someone on, say, the current extended-support release (52). Or safari on iOS 10, or Chrome on Android 8, or the default browser on a Samsung phone, etc. etc. etc.

In reality we we do support all of those, in the sense that if someone found a bug in one of them we would put effort into reproducing and fixing it. We just don't test them on ci. But that's just for practical reasons -- if we could test every one of the thousands of platforms we consider 'supported' in that sense we would; since we can't, we test a selection of common ones. But that doesn't mean the sdk isn't compatible with others.

ISTM what's relevant to the customer is what we support, in that sense (what we'll accept and fix bug reports on), not what we happen to test in ci (which we reserve the right to change at any point without notice). So I'd suggest making the list of what we support be that. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonWoolf I have a similar concern.
We still have to extra time on the CI (before we max out the allowed session length). So we could add some older browsers like:

  • Firefox 52
  • Chrome TDB
  • Safari iOS10
  • Chrome on Android 4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, we can discuss that in the CI issue. my main point for the purposes of this PR is that the libraries we support is a (large) superset of the libraries we test in ci, and we shouldn't imply otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that addressed with my comments re fallbacks?

Copy link
Member

@SimonWoolf SimonWoolf Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. ISTM that having a big table at the top saying "we're only compatible with firefox 58+, chrome on Android 4.0, ..." just because those are the versions we currently test in ci is just misleading in a way that isn't really anything to do with whether we have transport fallbacks (it's not like firefox 57 doesn't support websockets). Rather it's to do with whether we'll fix bugs reported against firefox 57. Which we will, even through we don't have that specific version in ci for practicality reasons. No?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @SimonWoolf's comments here. A visitor is probably not going to read the detail in reality, they are going to scan, see a table, and assume we don't support Opera or old versions, when we do.

@funkyboy funkyboy mentioned this pull request Mar 26, 2018
4 tasks
@funkyboy
Copy link
Contributor Author

Let's resume this conversation after we have added more browser tests on Browserstack

@mattheworiordan
Copy link
Member

Let's resume this conversation after we have

Should we not resolve this now and add more browsers once #493 is fixed. We should avoid coupling an update of the README to a task that will simply improve that README IMHO.

@funkyboy
Copy link
Contributor Author

@mattheworiordan @paddybyers @SimonWoolf ready for another look.

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

README.md Outdated

**Note**: the ably-js browser library contains fallback mechanisms in order to be able to support older browsers; specifically it supports comet-based connections for browsers that do not support websockets, and this includes JSONP for browsers that do not support cross-origin XHR.
Each of these fallback transport mechanisms is supported and tested on all of the listed target browsers; even when those browsers do not themselves require those fallbacks.
These mean that the library should be compatible with nearly any recent browser, on the majority of platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just remove "recent"

We still need to do this

@funkyboy
Copy link
Contributor Author

@paddybyers Ooops. Fixed now.

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@SimonWoolf
Copy link
Member

ready for another look.

I don't think my and Matt's issues with this approach (#481 (comment)) have really been addressed.

Rather than going over those again, I'll do my own version as a counterpoint and push it up to a branch, see what you think.

@SimonWoolf
Copy link
Member

@paddybyers
Copy link
Member

See #498 for my proposal for this

lgtm

@funkyboy funkyboy merged commit b841e60 into master Apr 5, 2018
@funkyboy funkyboy deleted the add-supported-platforms-to-readme-file branch April 5, 2018 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants